-
-
Notifications
You must be signed in to change notification settings - Fork 21.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Convert 3.x ESCN skeletons, animations, and shaders upon import #87106
base: master
Are you sure you want to change the base?
Conversation
These are several unrelated improvements, please make a PR for each feature (you do already have PRs for these open, these changes should be in one PR only unless the code in this depends on it, in that case please keep the details of that in its own PR and mention that this depends on that PR) |
That's the plan, they're not going to stay in this PR. This is just my working branch so I can get feedback on specific design questions. |
Thats not really how you should do things, each issue or feature should be discussed separately, and you shouldn't open a PR for your working branch but work on each feature on one branch unless they're dependent on each other 🙂, this just takes up processing and duplicates things |
632bf36
to
71c7ca0
Compare
Ok, I have removed the extraneous commits and specified which PRs this depends on. |
TokenStreamManip(const String &p_code) : | ||
old_code(p_code) { | ||
ShaderLanguage sl; | ||
sl.token_debug_stream(old_code, code_tokens, true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
token_debug_stream
I think this adds too much complexity to the import system which is not really the point of the importer. If the goal is to add back compat with 3.x resources, we should do it one of two ways:
My personal opinion is that if this is to be done automatically, it should be done in the resources itself, which you attempted to explain why it wasn't possible, but I think it should be made to be possible. For example, we have discussed a compatibility system. We know which Godot version the resources originated from, so perhaps we can add the missing parts to the resource loader so that we can adapt the properties. These would be a good usecase for such a compatibility system.
Yes, but you will know how much the index changes: you can store an offset that starts at 0 and increases by 2 for every transform track. In principle, the indices could be offset as the properties are set. It certainly is challenging and may be impossible without some changes to the core resource loader, but I do think we should be willing to look outside of the box rather than adding a lot of complexity to the import system.
What if when the code fails to compile, it could attempt to run it through the above converter and use that converted code if it succeeds?
Yeah I don't think we want to add compatibility code especially at runtime. Load-time or editor-time conversion could be supported only if TOOLS_ENABLED, for example. Do note also that if we do go this route, we need to make sure mesh compatibility issues are solved . related issues:
|
So what we talked about in the rocketchat was that the best course of action for modifying the resource loading to accommodate this would be to instead add For So for
This would still entail some steps in the scene importer; we would still have to recompute the animation frames to be absolute rather than relative to the skeleton rest, but we already do plenty of that in the scene importer for other scene types. For Shader, the main issues are:
For
So, these steps would only be run on Shaders that are embeded in other resources (i.e. would not automatically be run for standalone |
8323270
to
7dc2cbb
Compare
1bf5044
to
b0a9c87
Compare
0cf0acb
to
3e14c5e
Compare
3ebdfc0
to
d2b4da6
Compare
depends on #88078 |
018f1ed
to
7cc7deb
Compare
I have a project that contains ESCN, that needs this PR to make it in.... |
grab one of the editor artifacts from the build jobs, import it, and then save it as a godot 4 scene :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm under the weather, but I want some progress on this.
Some of the core io apis changed and I can't figure if they're necessary.
The changes are a bit invasive.
@clayjohn probably has opinions about the shader_converter.cpp
90d7e10
to
9134cee
Compare
9134cee
to
275104a
Compare
Depends on #88009, #88078
The Problem
ESCN was a discrete scene format that was created by the Godot team for ease-of-export from Blender to Godot. The plugin was originally written for Godot 3.x and used its scene format, and has not since been updated.
However, while Godot still has support for importing this format, ESCN importing has been broken since Godot 4.0 was released due significant changes in the scene format that conversion was not implemented for:
Transform pose
property which was relative to bone rest in 3.x , which was broken out toVector3 pose_position; Quaternion pose_rotation; Vector3 pose_scale
that are absolute in 4.x.Solution
This adds the following features to the ESCN editor importer:
transform
tracks into position, rotation, and scale tracks that are absolute rather than relative to the bone rest.Design Considerations
The reason this is a draft right now is because of the following issues that I need advice on. This will likely be broken up into seperate PRs based on the feedback.
ResourceLoader modification to allow special handlers for a single load
I had issues with converting Animations and Shaders; basically, we can't handle converting them in either of the
_set()
functions for these classes like we can forSkeleton3D
because:Animation tracks are stored ini style:
This means that, upon load, each of those indexed properties gets set, one at a time. We need to split the transformation track up into position, rotation, and scale tracks, and we can't arbitrarily add tracks in the middle of a load, since the index of the tracks will change.
Shaders compile their code upon setting the
code
property; if the code fails to compile,code
won't be set. Additionally, it will result in a very, very long error message that will obliterate the error logs and significantly confuse the user.Both of those class names are the same from 3.x to 4.x, so I couldn't implement another class for either of those to handle the loading either.
So, to solve these issues, I modified the ResourceLoader to allow the addition of special handlers. Basically, we can add a handler for a specified
Resource
class that takes in aMissingResource
and returns aResource
. If we try to load a resource, and the resource loader detects that it has a handler for this resource class, it will instead load the resource as aMissingResource
and, after the resource and its properties are finished loading, it will hand it off to the handler to instantiate the resource and set its properties.I had considered modifying the ResourceLoader classes to just add the property to
missing_resource_properties
if it fails to be set and then just getting those properties out of the metadata, but as mentioned above, this would result in a bunch of error messages (1000+ lines in total length) every time we tried to import an ESCN, which is not ideal.The way I have it now, I'm not really sure if this is the correct design; this sort of special handling needs to be only for a single load rather than a global handler, so I decided to only add it to the instantiated
ResourceLoader
classes rather than the staticResourceFormatLoader
classes. This breaks the pattern a bit, as it looks like it's intended that the user doesn't use the instantiatedResourceLoader
class and instead uses theResourceFormatLoader
class. I'd appreciate some advice on how to implement this such that it can be used for a single load and doesn't break the pattern here.ShaderLanguage token stream emitting
Converting Shaders from 3.x to 4.x is a bit more complicated than how it's currently handled in the 3.x to 4.x converter. In order to do a proper conversion of a 3.x to 4.x, we need to do the following:
SCREEN_TEXTURE
,DEPTH_TEXTURE
, andNORMAL_ROUGHNESS_TEXTURE
were removed, so their usage necessitates adding a uniform declaration at the top of the filevoid vertex()
tovoid process()
CLEARCOAT_GLOSS
was changed toCLEARCOAT_ROUGHNESS
and its usage was inverted (i.e. ascending values now increase the roughness rather than decreasing it). So, we need to invert all usages of CLEARCOAT_GLOSS:CLEARCOAT_GLOSS = 5.0 / foo;
CLEARCOAT_ROUGHNESS = (1.0 - (5.0 / foo));
,CLEARCOAT_GLOSS *= 1.1;
CLEARCOAT_ROUGHNESS = (1.0 - ((1.0 - CLEARCOAT_ROUGHNESS) * 1.1));
foo = CLEARCOAT_GLOSS;
foo = (1.0 - CLEARCOAT_ROUGHNESS);
async_visible
andasync_hidden
were removed, so these render modes need to be removedspecular_blinn
andspecular_phong
render modes were removed, and we can't handle these, so we just throw an errorMODULATE
is not supported (yet) in 4.x, so we have to throw an error.As you can see, this is a bit more complicated than a simple regex search and replace would allow for, in particular the
CLEARCOAT_GLOSS
replacement. We at least need a token stream in order to properly detect for these conditions and modify the code.So, in order to do this, I modified
ShaderLanguage
to emit a token stream:token_debug_stream()
was added to emit a parsed token stream for the entire file.position
andlength
members so that we can emit code based on the token stream; this is neccesary primarily because the token parser can modify both identifiers and constant values before creating the token.TK_TAB
,TK_CR
,TK_SPACE
,TK_NEWLINE
,TK_BLOCK_COMMENT
,TK_LINE_COMMENT
, andTK_PREPROC_DIRECTIVE
were added and_get_token()
was modified to emit them, but only if we are doing a debug parse. The reason for adding these was so that we can easily emit the new code from the token stream without obliterating the original formatting and commentsWith these modifications, we're able to easily detect for the above conditions and insert and remove tokens from the token stream, and then emit code based on that token stream.
I'm a bit more confident about how I modified ShaderLanguage here but I'd still like to get feedback on how I modified ShaderLanguage here, and if there are any other 3.x to 4.x differences that I need to handle that I missed.